[DirectX] Write DXIL with debug info to ILDB part by kuilpd · Pull Request #158 · access-softek/llvm-project

kuilpd · 2026-06-02T17:36:56Z

Internal review before creating this PR in the upstream.

hvdijk · 2026-06-02T17:43:12Z

+          for (unsigned I : seq(FlagEntries.size())) {
+            llvm::Module::ModuleFlagEntry &Entry = FlagEntries[I];


I may be missing something but why isn't this simply

Suggested change

for (unsigned I : seq(FlagEntries.size())) {

llvm::Module::ModuleFlagEntry &Entry = FlagEntries[I];

for (llvm::Module::ModuleFlagEntry &Entry : FlagEntries) {

?

hvdijk · 2026-06-02T17:45:18Z

+              continue;
+            }
+            M.addModuleFlag(Entry.Behavior, Entry.Key->getString(),
+                            cast<ConstantAsMetadata>(Entry.Val)->getValue());


Do we know that Entry.Val is always a ConstantAsMetadata?

I've copied this loop from DXC's code, not sure how else to do this.

addModuleFlag has an overload that takes a Metadata* which seems like it should work.

hvdijk · 2026-06-02T17:49:00Z

+        replaceNamedMetadataArray(M, "dx.source.args", {});
+      } else {
+        // If we have an ILDB part, strip DXIL from all debug info.
+        StripDebugInfo(M);


This function returns a bool indicating whether any debug info was stripped, we can use this to avoid checking through debug_compile_units().empty() and get a more reliable answer.

I think we specifically need to strip debug info only if we already know if there is debug info. Is there any other way to know this?

This function returns a bool indicating whether any debug info was stripped, we can use this to avoid checking through debug_compile_units().empty() and get a more reliable answer.

AFAIK if !llvm.dbg.cu is empty, LLVM assumes no debug info is present in the file (see #150 (comment)). Are you aware of cases when we have correct and full LLVM IR with debug info but without compile units?

There is no harm in stripping debug info if there is no debug info, the function will just tell you there was nothing to strip, and it will have done a better search than the current code. So something like std::unique_ptr<Module> Clone = llvm::CloneModule(M); bool HaveDebugInfo = llvm::StripDebugInfo(*Clone);, then write the DXIL section from *Clone (which assuredly has no debug info, regardless of whether there was originally debug info), then if HaveDebugInfo is true, create your second clone and write the ILDB section from that.

There is no harm in stripping debug info if there is no debug info

But it performs an extra pass over the whole module which can be avoided when shader is compiled without debug info.

(Anyway, my opinion on that matter should not block this PR.)

The thing is, that if we have no compile units, but still have debug info nodes, it may be an issue somewhere in LLVM IR producers, or in an LLVM IR pass which runs before DXILWriterPass.

It may be, sure, but...

By calling strip on such IRs, DXILWriterPass will just hide such an issue.

...the issue won't be hidden: it will accurately write a DXIL section without debug info, and an ILDB section with the weird debug info.

I'd be fine with turning this into an assert instead somewhere, but silently leaving debug info in the section that specifically is meant to not have debug info seems wrong, especially if we don't know if there may be legitimate reasons for it that we're just not thinking of now.

Definition DISubprograms must have compile unit field, which is enforced by Verifier.

Note that I didn't say no DICompileUnit, just no !llvm.dbg.cu named metadata. There are loads of LLVM tests that have a DICompileUnit that is not linked through named metadata, a random example is llvm/test/Transforms/Coroutines/declare-value.ll.

Definition DISubprograms must have compile unit field, which is enforced by Verifier.

Note that I didn't say no DICompileUnit, just no !llvm.dbg.cu named metadata. There are loads of LLVM tests that have a DICompileUnit that is not linked through named metadata

Such IRs are definitely incorrect from the Verifier's point of view - see Verifier::verifyCompileUnits() - it discards debug info containing compile units that are not listed in !llvm.dbg.cu,
For the test you referenced:

DICompileUnit not listed in llvm.dbg.cu !4 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !5, isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug) llvm-dis.exe: warning: ignoring invalid debug info in tmp.bc

Tests from llvm\test\Transforms sometimes do not care about that - they run a single pass, not full llc pipeline (and opt silences invalid debug info warning). So it may have happened that there they've been unverified.

By calling strip on such IRs, DXILWriterPass will just hide such an issue.

...the issue won't be hidden: it will accurately write a DXIL section without debug info, and an ILDB section with the weird debug info.

It may be easier to spot a problem when debug info section requested by the user is not created than when it is created but later debug info is discarded by IR consumers. Usually, IR loader does not raise an error if debug info is incorrect - it silently strips it (or it raises a warning if user explicitly invokes llvm-dis, but not an error).

I'd be fine with turning this into an assert instead somewhere, but silently leaving debug info in the section that specifically is meant to not have debug info seems wrong, especially if we don't know if there may be legitimate reasons for it that we're just not thinking of now.

That's definitely worth doing if we leave unconditional Strip call 👍
We could also do it the other way around: detect debug info presence by checking compile units, and add an assert(StripDebugInfo(M) == HasDebugInfo && "This module must not contain anything to be stripped")

So, what's the consensus? Do we change the check !M.debug_compile_units().empty() to a call to StripDebugInfo with an assert?

I won't be blocking it if we use Strip as the source of truth here, but an extra assert for !M.debug_compile_units().empty() value would be nice :)
Motivation: other passes (either DX-specific or generic IR passes) may check compile units list still, to check if debug info should be processed, and they won't be calling StripDebugInfo wherever they want to check for debug info presence. So we want to be sure that both debug info presence indicators converge.

The details of how you do it are up to you, but what matters to me is that if we have debug info, even weird debug info, we don't enter the "no debug info" case, at least in an assertions-enabled build. Checking the result of StripDebugInfo seemed to me like the simplest way of achieving that, but any other way that also achieves that same goal works for me.

dzhidzhoev

llvm/test/CodeGen/DirectX/embed-ildb.ll from dx-debug has lines for testing dx.source presence in output sections:

ILDB-DIS: !dx.source.contents
ILDB-DIS: !dx.source.defines
ILDB-DIS: !dx.source.mainFileName
ILDB-DIS: !dx.source.args
...
+; DXIL-DIS-NOT: !dx.source

Could you please import that lines into this PR?
So that there will be test coverage for changes made to !dx.source processing in this PR.

dzhidzhoev · 2026-06-02T17:38:37Z

    // The DXIL part also writes a program header, so we need to include its
    // size when computing the offset for a part after the DXIL part.
-    if (Sec.getName() == "DXIL")
+    if (Sec.getName() == "DXIL" || Sec.getName() == "ILDB")


Could we use dxbc::isProgramPart()? If it's not merged into mainline, let's do it as well.

dzhidzhoev · 2026-06-02T17:39:28Z

    uint64_t PartSize = SectionSize;

-    if (Sec.getName() == "DXIL")
+    if (Sec.getName() == "DXIL" || Sec.getName() == "ILDB")


dzhidzhoev · 2026-06-02T17:39:35Z

    PartSize = alignTo(PartSize, Align(4));
    W.write<uint32_t>(static_cast<uint32_t>(PartSize));
-    if (Sec.getName() == "DXIL") {
+    if (Sec.getName() == "DXIL" || Sec.getName() == "ILDB") {


dzhidzhoev · 2026-06-02T17:44:23Z

+}
+
 class EmbedDXILPass : public llvm::ModulePass {
+  std::string writeModule(Module &M, bool HasDebugInfo, bool IsDebug) {


Let's rename these variables: HasDebugInfo -> ShouldStripDebug , IsDebug -> WriteDebugPart, or something like that. So that their purpose will be less confusing for readers.

dzhidzhoev · 2026-06-02T17:46:24Z

+      }
+    }
+
+    const auto DIMap = DXILDebugInfoPass::run(M);


Let's move this under under if (IsDebug) above, following the email discussion we had (Andrew's email).

The call to WriteDXILToFile requires a DXILDebugInfoMap argument though, I don't think we can not call it. Doesn't seem to do any harm.

@hvdijk mentioned that we can pass an empty map.

Like this?

const DXILDebugInfoMap DIMap; WriteDXILToFile(M, OS, DIMap);

I tried, it crashes everything.

It should be like

const DXILDebugInfoMap DIMap; ... if (IsDebug) { ... DIMap = DebugInfoPass::run(M); } else { ... // No DebugInfoPass::run(M); } ... WriteDXILToFile(M, OS, DIMap); ... }

Does it crash in such case as well?

No, it means we may have an intermediate stage where both parts are emitted with debug info.

Ah, I see, that is fine for what gets merged into LLVM main but that means the stacked PR approach won't work for the "strip debug info from DXIL" part of it. I think that means we won't be able to create that PR yet to get that reviewed, because it depends on multiple other PRs that are not otherwise related and GitHub does not support that. If it's fine with you to not have that part reviewed just yet, that's also fine with me.

No, it means we may have an intermediate stage where both parts are emitted with debug info.

Ah, I see, that is fine for what gets merged into LLVM main but that means the stacked PR approach won't work for the "strip debug info from DXIL" part of it. I think that means we won't be able to create that PR yet to get that reviewed, because it depends on multiple other PRs that are not otherwise related and GitHub does not support that. If it's fine with you to not have that part reviewed just yet, that's also fine with me.

Well, we could try sending dx.ildb PR with !M.debug_compile_units().empty() check to determine debug info presense (that's what I implied when I told dx.ildb change does not depend on Strip changes, sorry), and the assert for checking that "StripDebugInfo() did not find any debug info when there are no compile units" could be a part of dx.dxil change.

Yes, if we choose the approach to check debug info presence mainly with StripDebugInfo, then changes for both sections depend on StripDebugInfo call correctness.

it depends on multiple other PRs that are not otherwise related and GitHub does not support that

AFAIK if we choose this option from https://llvm.org/docs/GitHub.html#stacked-pull-requests:

Two PRs with a dependency note

Create PR_1 for feature_1 and PR_2 for feature_2. In PR_2, include a note in the PR summary indicating that it depends on PR_1 (e.g., “Depends on #PR_1”).

To make review easier, make it clear which commits are part of the base PR and which are new, e.g. “The first N commits are from the base PR”. This helps reviewers focus only on the incremental changes.

We can make a PR which depends on two other PRs.

When I rebased this PR on your hvdijk/llvm-project@directx-delay-converting-debug-info, it all worked. The only problem is that I had to rebase all other commits that have been merged to main since you created those branches (including some DX patches required for this PR). If you rebase your branches on current upstream main, I will be able to stack this PR on your new #201336, and we won't have to split anything.

It needs to be a merge rather than a rebase so that the PRs don't get messed up, but sure, done.

Write DXIL with debug info to ILDB part

b495ef3

kuilpd requested review from dzhidzhoev and hvdijk June 2, 2026 17:36

hvdijk reviewed Jun 2, 2026

View reviewed changes

dzhidzhoev reviewed Jun 2, 2026

View reviewed changes

kuilpd and others added 4 commits June 2, 2026 23:15

Redo the loop that removes version metadata

28108be

Add dx.source checks to embed-ildb.ll test

b13ae6d

Introduce isProgramPart

5560932

Rename IsDebug to WriteDebug

0281db9

kuilpd requested a review from asavonic June 2, 2026 22:24

		for (unsigned I : seq(FlagEntries.size())) {
		llvm::Module::ModuleFlagEntry &Entry = FlagEntries[I];

	for (unsigned I : seq(FlagEntries.size())) {
	llvm::Module::ModuleFlagEntry &Entry = FlagEntries[I];
	for (llvm::Module::ModuleFlagEntry &Entry : FlagEntries) {

Conversation

kuilpd commented Jun 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzhidzhoev Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzhidzhoev Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzhidzhoev Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzhidzhoev Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzhidzhoev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzhidzhoev Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzhidzhoev Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

dzhidzhoev Jun 2, 2026 •

edited

Loading

dzhidzhoev Jun 2, 2026 •

edited

Loading

dzhidzhoev Jun 3, 2026 •

edited

Loading

dzhidzhoev Jun 3, 2026 •

edited

Loading

dzhidzhoev left a comment •

edited

Loading

dzhidzhoev Jun 3, 2026 •

edited

Loading

dzhidzhoev Jun 3, 2026 •

edited

Loading